Conversation
|
This is somewhat quick-and-dirty solution. Perhaps, we we shouldn't run |
WGH-
force-pushed
the
support-gitattributes
branch
from
cd10de9 to
86ef851
Compare
There was a problem hiding this comment.
Overall the approach here looks totally sensible, thanks for taking the time to write a PR!
I do think it needs a test: perhaps the easiest thing would be to write a unit test that e.g. "package.json\0diff\0unspecified\0" parses to the value you expect, and use that function after getting the raw string from git check-attr.
Otherwise, I think we should support at least the binary attribute too, and this is good to go. Thanks again, and let me know if you need any help :)
|
|
||
| /// Corresponds to the diff attribute. See man gitattribute. | ||
| pub(crate) enum DiffAttribute { | ||
| Set, |
There was a problem hiding this comment.
Would you mind adding doc comments to these? Even a brief phrase copied from the man page would be a big help. I think Other is something you've added, and isn't defined in git?
| "set" => Self::Set, | ||
| "unset" => Self::Unset, | ||
| "unspecified" => Self::Unspecified, | ||
| _ => Self::Other, |
There was a problem hiding this comment.
I guess the other option would be to use TryFrom so you don't need this catch-all Other value.
There was a problem hiding this comment.
I suppose this is no longer relevant since I changed this variant to Other(String)
| } | ||
| } | ||
| Err(err) => { | ||
| warn!("failed to execute git: {err}"); |
There was a problem hiding this comment.
These should be debug logging I think, especially this one. You can run difft in a directory that isn't a git repository, or even on a machine that doesn't have git installed.
There was a problem hiding this comment.
Running git outside of git repository is already handled by output.status.success() check, which gets logged with debug level.
I erroneously claimed that git being unavailable also goes there, but yeah, it actually ends up as a warning here.
Changed it to debug.
| /// Runs `git check-attr diff` to get the diff attribute of the path. Returns | ||
| /// [`Option::None`] when either `git` is not available, file is not inside git | ||
| /// directory, or something else went wrong. | ||
| pub(crate) fn check_attr(path: &Path) -> Option<DiffAttribute> { |
There was a problem hiding this comment.
I think should either be check_diff_attr, or take an attribute name as an argument.
There was a problem hiding this comment.
I changed this to check_diff_attr. It also checks binary attribute under the hood, but I decided against complicating thing to the caller too much
| let (mut lhs_src, mut rhs_src) = match ( | ||
| guess_content(&lhs_bytes, lhs_path, binary_overrides), | ||
| guess_content(&rhs_bytes, rhs_path, binary_overrides), | ||
| check_attr(Path::new(display_path)), |
There was a problem hiding this comment.
This covers the case when someone writes *.ext -diff, but not *.ext binary, which is also mentioned in #466.
Apparently binary means -text -diff -merge according to https://git-scm.com/docs/gitattributes#_using_macro_attributes
I think we should support -text and binary too, and also treat them as forcing binary.
There was a problem hiding this comment.
Yeah, that's probably a good idea, since specifying both binary and an external diff driver with diff= is valid.
There was a problem hiding this comment.
To avoid calling git check-attr twice, and to hide this complexity in main, I decided to make binary essentially force -diff. Please tell if you're against this implicit behaviour
There was a problem hiding this comment.
Alternatively, we could return something like Option<(DiffAttribute, BinaryAttribute)>. That would be more explicit, but the patterm matching expression in main is already complicated as it is, and it will become even more ridiculous.
I don't really have a strong preference here. I can implement it either way, please tell me, how :)
WGH-
force-pushed
the
support-gitattributes
branch
from
86ef851 to
1fcd21d
Compare
WGH-
force-pushed
the
support-gitattributes
branch
3 times, most recently
from
246876e to
a120746
Compare
Runs git check-attr to get the files git attributes, and treats the file as binary if the attribute value is "unset", matching the built-in git diff. Fixes Wilfred#466 Idea-by: @anuramat (Wilfred#466 (comment))
WGH-
force-pushed
the
support-gitattributes
branch
from
a120746 to
982edb0
Compare
Wilfred
left a comment
Wilfred
left a comment
There was a problem hiding this comment.
Almost there! The logic looks correct to me, but it's really confusing that Unset means different things for the text and binary attributes. Could you name them say AssumeText and AssumeBinary instead?
2 participants
Runs
git check-attrto get the files git attributes, and treats the file as binary if the attribute value is"unset", matching the built-in git diff.Fixes #466
Idea-by: @anuramat (#466 (comment))